Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable unmarshaling partitioned caches #456

Closed
wants to merge 1 commit into from

Conversation

chlowell
Copy link
Collaborator

@chlowell chlowell commented Sep 6, 2023

Closes #455 by adding AdditionalFields and JSON tags.

@sonarcloud
Copy link

sonarcloud bot commented Sep 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

IDTokensPartition map[string]map[string]IDToken
AccountsPartition map[string]map[string]shared.Account
AppMetaData map[string]AppMetaData
AccessTokensPartition map[string]map[string]AccessToken `json:"AccessTokensPartition,omitempty"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgavrilMS is there a schema for partitioned caches or an example of how one should look serialized? I didn't find any, so I don't know the correct keys here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serialized partitioned cache should look exactly like the non-partitioned cache. The partitioned cache is just an implementation detail in MSAL, so that MSAL is able to perform fast look-up.

You do this by going through the entire double dictionary and fetching the tokens. The first "key" is the partition key (ignore it) and the second key is the access token key - identical to the non-partitioned cache.

E.g. for an app access token you can have:

partition key: clientId + tenantId1
access token key: clientId + tenantId1 + resource1 and clientId + tenantId1 + resource2

partition key: clientId + tenantId2
access token key: clientId + tenantId2 + resource1 and clientId + tenantId2 + resource2

But in the serialized cache you will have only the 4 tokens with their access token keys.

@pmaytak can help here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that MSAL has a collection of partitions, each partition is indistinguishable from a non-partitioned cache, and MSAL should de/serialize only one partition at a time? That is to say, the partition key is an input to de/serialize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, partition key is a cache entry key to de/serialize.

Something like this:
image

See app cache implementation in MSAL.NET

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSAL Go partitions data in memory as expected however it always de/serializes all partitions. There's no way to de/serialize a single partition. Let's not proceed with this PR then, much more is required to make persistent caching work correctly with partitions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSAL.NET also de/serializes all partitions (so full internal cache). However, an L2 read only ever loads one partition. So as long as CCA is created per request, there's only ever 1 partition in the cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing something here. MSAL Go gives the storage implementation opaque bytes and a suggested partition key, implying the bytes represent a single partition. If MSAL always de/serializes all partitions, a storage implementation that divided the cache across physical storage according to MSAL's suggested keys would just create redundant copies of the entire cache, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If MSAL always de/serializes all partitions, a storage implementation that divided the cache across physical storage according to MSAL's suggested keys would just create redundant copies of the entire cache, no?

Yes, if MSAL instance (with the internal cache) is used as a singleton - then the external cache entries can have duplicate data. That's why we tell to create a new confidential client instance per request, so that the cache is empty initially and is deserialized with one partition.

gives the storage implementation opaque bytes and a suggested partition key, implying the bytes represent a single partition

This should be how it works, but MSAL.NET's implementation was not ideal. Ideally the serializer will accept a cache key, and try to serialize only that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't unmarshal a partitioned cache
3 participants